fix(ci): three correctness fixes from persona review of PR #25#26
Merged
Conversation
Found by ce-correctness-reviewer in session e8389343 (dispatched against PR #25's diff as the work-dt42 supersession spike test). Three low- severity issues addressed; one (dispatch-by-SHA) skipped because the API doesn't support it. 1. Branch-lookup fallback uses run.head_sha (was: pr.head.sha) When workflow_run fires after a workflow_dispatch-triggered ci.yml run, run.pull_requests is empty and we fall through to pulls.list by branch. Previously we built candidates with `headSha: pr.head.sha` (CURRENT branch HEAD) instead of run.head_sha (the SHA ci.yml actually ran against). On a HEAD-moved race the moved-HEAD guard (cand.headSha !== pr.head.sha) would silently pass, and we'd log a misleading "required check not green yet" when the real cause was the SHA mismatch. Functionally safe before (the check_runs query for the new SHA wouldn't find a green run), but the diagnostic now correctly identifies the race. 2. Skip-and-retry when pr.mergeable is null GitHub returns mergeable=null when it hasn't finished computing mergeability (common right after a push or PR open). Previously we treated null as "proceed" and let pulls.merge fail with a 405, which the outer try/catch swallowed as a warning. The next workflow_run would retry. The new explicit `mergeable === null` branch logs "still computing; will retry on next workflow_run" instead of the bogus merge-failed warning. Same eventual outcome, cleaner logs. 3. checks.listForRef filters server-side by check_name Was: per_page=100 with client-side filter. If >100 check_runs ever accumulate on a SHA (re-runs, third-party checks, etc.), the green run could fall past the first page and be silently invisible. The `check_name` query parameter scopes the API response to one name server-side; pagination ceiling can never hide our run. NOT applied: fix #4 (dispatch ci.yml against SHA instead of branch name). The persona suggested createWorkflowDispatch accepts a SHA in `ref`, but GitHub's REST API documents `ref` as "branch or tag name" only — SHA isn't supported. The race window (branch HEAD moves between event delivery and dispatch resolution) is small and Phase 2's moved-HEAD guard catches the resulting SHA mismatch. Documented in a comment block near the createWorkflowDispatch call. All three fixes were graded low-severity by the persona; fix #3 has the worst long-tail failure mode (silent infinite-skip if the truncation ever happens). nix flake check green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ce-correctness-reviewer(dispatched on PR #25's diff via the persona-resolver pattern, as the work-dt42 supersession spike) surfaced four low-severity correctness findings in.github/workflows/auto-merge-publish-dist.yml. Three are fixed here; one (dispatch-by-SHA) is skipped with a comment explaining why.Fixes
Branch-lookup fallback uses
run.head_sha(was:pr.head.sha) — whenworkflow_runfires after aworkflow_dispatch-triggeredci.ymlrun,run.pull_requestsis empty and we fall through topulls.listby branch. Usingpr.head.sha(current branch HEAD) instead ofrun.head_sha(the SHAci.ymlactually ran against) meant the moved-HEAD guard silently passed on force-pushes and logged a misleading "check not green yet" when the real cause was the SHA mismatch. Functionally safe before (the check_runs query for the wrong SHA wouldn't find a green run); now the diagnostic correctly identifies the race.Skip-and-retry when
pr.mergeableisnull— GitHub returnsmergeable: nullwhen it hasn't finished computing mergeability yet. Previously the strict=== falsecheck treatednullas proceed, the merge call failed with 405, and the outer try/catch swallowed it as a warning. New explicitnullbranch logs "still computing; will retry on next workflow_run" instead of the bogus failure. Same eventual outcome; cleaner logs.checks.listForReffilters server-side bycheck_name— was:per_page: 100with client-side filter onr.name === REQUIRED_CHECK. If >100 check_runs ever accumulated on a SHA, the green run could fall past page 1 and be silently invisible (infinite-skip loop). Thecheck_namequery parameter scopes the API response to one name server-side; pagination can never hide it.Not applied: fix #4 (dispatch by SHA)
The persona suggested
createWorkflowDispatchaccepts a SHA inref. It doesn't — GitHub's REST API documentsrefas "branch or tag name" only. The race window (branch HEAD moves between event delivery and dispatch resolution) is small and Phase 2's moved-HEAD guard catches the resulting SHA mismatch correctly. Documented in a comment block near thecreateWorkflowDispatchcall.Test plan
nix flake checkgreen (treefmt + actionlint × 4 workflow files + 167 pytest)publish-distauto-PR. That'll happen on the next upstream EveryInc release or a deliberate converter edit.Provenance
Findings from session
e8389343-9c4f-4b7e-9699-7ca53e4fb349, in which a singlece-correctness-reviewerdispatch on PR #25's diff served simultaneously as: (a) the work-dt42 supersession spike (which it cleared — see closed beadwork-dt42for the "Tier 1 supersedes" reasoning), and (b) a real-diff correctness review of the just-shipped workflow.work-dt42 closed; this PR captures the bonus output.